Conversation
6556114 to
394df53
Compare
|
Hi maintainers, I pushed a small follow-up docs commit (9b8867a) that adds a direct README link to the bot platform guide and corrects the verification metadata date. The new CI run is currently marked action_required with no jobs, so it appears to be waiting for workflow approval for this fork PR. Could a maintainer approve the workflow and take a look when convenient? The PR remains documentation-only and contains no credentials or raw provider payloads. |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 9b8867a10eee1e40534b2b3c17d72dc2b3ff8130. This docs-only change adds the bilingual bot-platform capability guide and its index links. The guide currently overstates the runtime's allowlist and per-channel proxy contracts, and one capability row omits important conversation-scope limits. Technical NO-GO until the P1/P2 claims are corrected.
Validation completed: build:test, full typecheck, lint, format check, ASF header check, diff check, and 102 focused Core/Runtime bot tests. I also ran compiled production-mapper and credential-probe checks, verified all local documentation targets, and confirmed a clean merge with current main ea990cab7ffa768dc1a574b539f73a83f8faabb8. GitHub exposes no hosted checks for this head. I did not independently repeat the author-reported live provider smokes. Seven external setup links returned HTTP 200; the DingTalk documentation hostname did not resolve from this runner.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
|
||
| ### Restrict who can contact a bot when the deployment needs it | ||
|
|
||
| `allowedUserIds` is an optional per-channel allowlist. When it is absent or |
There was a problem hiding this comment.
[P1] Do not promise an allowlist on bridges that never enforce it
This is a security-boundary claim that is false for five of the eight bridges. BotRegistry.wire() forwards every bridge message event without a central filter, and only Telegram, Feishu, and WeCom consult allowedUserIds. On this exact build, a compiled production-mapper probe with allowedUserIds: ['trusted-user'] still produced inbound events for attacker-not-in-allowlist on Discord, DingTalk, QQ, Slack, and WeChat; Feishu and WeCom control probes returned null. The settings UI also exposes this field only for Telegram. A user following this section can therefore believe untrusted senders are blocked while those messages still enter a Session. Either enforce and expose the allowlist for every channel claimed here, or explicitly limit this contract to the providers that implement it. Please update the Chinese guide in the same change.
|
|
||
| 1. Use [BotFather](https://core.telegram.org/bots#how-do-i-create-a-bot) to | ||
| create a bot with `/newbot` and copy its token. | ||
| 2. Put the token in Maka's `token` field. Set `proxyUrl` only when the Telegram |
There was a problem hiding this comment.
[P2] The channel proxy field is not used by either credential path
Neither Telegram nor Discord consumes BotChannelSettings.proxyUrl. All bot HTTP calls go through proxiedFetch(), which resolves only the active global network proxy, and the runtime bot directory has zero reads of the channel field. In an exact-head probe with the global proxy disabled and each channel's proxyUrl pointed at a local listener, both credential tests contacted the real provider endpoints while the listener observed zero connections. Thus this instruction, and the Discord claim that the channel setting covers authentication, send users toward a setting that currently covers nothing. Wire the field into the relevant transports, or document the actual global/system proxy requirement instead; mirror the correction in the Chinese guide.
|
|
||
| | Channel | Normal conversation reply | Scheduled bot delivery | Progressive reply stream | Typing indicator | Ephemeral reply cleanup | Non-text message classification | | ||
| | --- | --- | --- | --- | --- | --- | --- | | ||
| | Telegram | Yes | Yes | Yes | Yes | Yes | Yes | |
There was a problem hiding this comment.
[P3] Qualify the conversation scope of these “Yes” capabilities
The current table reads as channel-wide support, but TelegramBotBridge.startReplyStream() returns null for every group conversation, and QQ's typing endpoint is implemented only for channel: (guild-channel) targets, not QQ groups or C2C chats. Normal replies still work, so this is non-blocking, but a capability guide should label Telegram progressive streaming as private-chat only and QQ typing as guild-channel only in both language versions.
|
Thanks for the review. Addressed in commit 334b215:
The new CI run is currently |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 334b215e9952daef9a3def4f970d72fd9cdf101b. This follow-up correctly narrows the allowedUserIds security contract, removes the unsupported channel-level proxyUrl claim, and adds the missing Telegram/QQ conversation scopes. The replacement global-proxy guidance still does not match the production Bot path, so the technical conclusion remains NO-GO due to the P2 inline finding.
Validation completed: build:test, full typecheck, lint, format check, ASF header check, diff check, 102 focused Core/Runtime bot tests, and 93 relative documentation links. A compiled exact-head Desktop settings probe also confirmed that applying an enabled global proxy leaves the Bot transport proxy state unset. The head cleanly merges with current main c22768c3b0dc47518f6f8584e864f86f0b1e5379. GitHub exposes no hosted checks for this head. I did not repeat live-provider smoke tests or native Windows/macOS networking.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| Runtime consumes; it is not a claim that the credentials have been validated | ||
| against a live provider. | ||
|
|
||
| Bot HTTP requests currently use the active global network proxy resolved by |
There was a problem hiding this comment.
[P2] Do not direct Bot users to the global proxy until the BotRegistry is actually wired to it. proxiedFetch() only reads the module-local activeProxy, but no production path calls setActiveProxy(): Desktop constructs BotRegistry and its settings effect forwards only botChat. A compiled exact-head probe applied an enabled settings.network.proxy and still observed resolveActiveProxy() === null. SDK-backed paths are also outside this claim: Slack uses WebClient directly, and a credential/bridge probe made zero connections to the configured local proxy. As written, users behind a proxy will follow this guide and still send Bot HTTP traffic directly or fail to connect. Either plumb the Host-owned proxy snapshot into every Bot transport, including SDK-backed bridges, or document that Bot proxy routing is not currently supported.
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
8dc844f to
57caebc
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 57caebc06d986f3b3960d6f37332669a2491af16.
This follow-up connects Desktop-owned bot HTTP traffic to the Host-resolved network proxy, adds proxy-resolution IPC and retry handling, and updates the platform guide. I found two high-impact proxy-boundary regressions and one remaining transport/documentation mismatch, so this is not ready to merge.
Validation: build:test; full typecheck, lint, format, ASF-header and diff checks; Desktop 2503/2503; Runtime 3465 pass / 13 skip; focused proxy tests 23/23. Runtime Host completed with 1937 pass / 19 skip / 2 fail: the existing managed-Bash sandbox integration is blocked by this environment, and an unchanged endpoint-probe timing test passed on immediate focused rerun. The branch cleanly merges with current main (c22768c3b0dc47518f6f8584e864f86f0b1e5379). GitHub currently reports no hosted checks. I did not test real external bot providers or native Windows/macOS networking.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| lastReportedError = undefined; | ||
| attempt = 0; | ||
| apply(resolved.kind === "ready" ? (resolved.proxy ?? null) : null); |
There was a problem hiding this comment.
[P1] Keep an authenticated-proxy failure fail-closed. The protocol deliberately distinguishes credential_not_configured from a ready result with no proxy, but this line maps both to setActiveProxy(null). In an exact-head production-path probe, resolving credential_not_configured and then calling proxiedFetch produced active=null, one direct origin connection, and zero proxy connections. Thus a missing or unreadable proxy secret silently sends bot HTTP outside the configured network boundary; Host WebFetch, WebSearch, and external-agent installation reject the same state instead. Preserve a blocked proxy state (or otherwise make client-owned requests fail) until credentials are restored, and add a regression that proves no direct dial occurs.
| // A non-local Host describes a different machine's network. The bot | ||
| // bridges dial out from this one, so its proxy policy does not apply and | ||
| // guessing would be worse than staying direct. | ||
| if (deps.profileKind !== "local") { |
There was a problem hiding this comment.
[P1] Do not let every connected Host profile mutate one process-global proxy. Client IPC is registered for every owner target, while every remote/environment applier clears the global state here; scheduled retries also have no dispose or generation fence when a candidate retires. With two production appliers, a remote refresh cleared the local proxy and a delayed local resolution later reapplied it (remote:null then local:127.0.0.1). Because Desktop keeps multiple profiles connected and uses one process-wide BotRegistry, enabling, reconnecting, or retiring another profile can silently bypass the selected local proxy or restore a stale one. Coordinate the state against the authoritative active/default target and cancel or fence retired appliers.
| Runtime consumes; it is not a claim that the credentials have been validated | ||
| against a live provider. | ||
|
|
||
| Bot HTTP requests currently use the active global network proxy resolved by |
There was a problem hiding this comment.
[P2] This statement is still false for Slack HTTP traffic. SlackBotBridge and testSlack construct @slack/web-api WebClient instances without its supported custom fetch option, so the SDK uses globalThis.fetch, not Maka proxiedFetch. With an active proxy and a local Slack API fixture, auth.test() hit the origin directly once and the proxy zero times. Slack authentication, Socket Mode URL acquisition, and message sends therefore still bypass the configured proxy. Inject the proxied fetch into the Slack clients (including the Socket Mode client options) or narrow this documentation.
hqhq1025
left a comment
There was a problem hiding this comment.
The prior credential, cross-profile race, and Slack HTTP findings are addressed, but two process-wide proxy lifecycle paths remain unsafe. The selected/default Host can leave Bot traffic fail-open while its policy is unresolved, and an unrelated non-default Host cleanup can globally block the healthy default route. These need production-path lifecycle coverage before the documented global proxy behavior is reliable.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| }, | ||
| onDefaultProfileChanged: (profileId) => { | ||
| void clientNetworkProxyAppliers |
There was a problem hiding this comment.
[P1] Block client traffic until the authoritative proxy policy is known
This optional refresh leaves the process-global proxy unchanged when the newly selected default has no ready applier. The same window exists during initial registration because the refresh is fire-and-forget while active-proxy-state starts as unblocked/direct. Using the compiled production components, I held an authoritative local resolution pending and proxiedFetch reached a loopback origin directly; selecting an unavailable default with no map entry also left blocked=false and reached the origin. If that Host requires an authenticated proxy, bot auth/send traffic can bypass the policy before resolution or during retries. Set the process state to blocked synchronously when authority changes or is first attached, and only unblock it after an explicit ready direct/proxy result.
| clientNetworkProxyAppliers.delete(profileId); | ||
| // No replacement target owns the process-wide transport anymore. Keep | ||
| // client-owned requests fail-closed until the default target returns. | ||
| setActiveProxyBlocked(); |
There was a problem hiding this comment.
[P2] Do not block the active route when a non-default Host closes
Every owner target gets its own map entry, so this condition is also true for a non-default candidate. In a compiled lifecycle probe, the local default applier remained registered, but cleaning up an unrelated remote target executed this branch, set the global state to blocked, and made proxiedFetch reject before the origin saw a request. That takes all Bot HTTP offline until some unrelated default refresh occurs. Only clear/block when the closing applier is the authoritative default, or immediately reapply the current default owner.
# Conflicts: # packages/runtime-host/src/protocol/index.ts
Summary
Refs #3894.
Adds synchronized English and Chinese documentation for the eight current bot platforms, including a source-derived capability matrix, provider setup guidance, security boundaries, limitations, and a repeatable live-validation checklist. This is a documentation-only change; it does not change runtime behavior.
Verification
git diff --check: passed@Makareply, content preservation, and post-restart group reply passed@Makaround trip and exact reply content passed; a system-level TUN route was required for the Gateway WebSocketcontext_tokenand sends an empty token for outbound replies, so the documentation records this implementation risk instead of claiming live support.No credentials, user IDs, or raw provider payloads are included. Restart recovery, duplicate suppression, group behavior where not tested, and scheduled delivery remain untested in the live smoke entries.
AI use
Tool(s) and scope:
Codex was used for source review, documentation drafting, platform smoke-test recording, and verification.
Checklist
Does this PR entail a change in behavior?